-
Notifications
You must be signed in to change notification settings - Fork 21.2k
Block access list changes - BAL construction, execution and validation #32263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
A lot of the comments in the code are straight-up wrong/misleading. There's a lot of notes/reminders I made as I was implementing this, and not all of them have been removed/corrected at this point. |
I've pushed parallel execution changes here. Still failing 2 tests (other than modexp repricing ones which will be fixed with a rebase on master):
I've been trying to debug these but it's proving to be exceedingly difficult with the parallel execution enabled. The tests seem to relate to behavior of some system contracts on the fork boundaries, so I will just proceed with gathering numbers on mainnet performance for the meantime. |
ab1c562
to
8f200db
Compare
Broke the block count on the insertion log statement here. Will try to fix tomorrow. |
Getting some empty accounts in the BAL. example:
|
…hen enabled, post-Cancun blocks which lack access lists will have them constructed on execution during import. When importing blocks which contain access lists, transaction execution and state root calculation is performed in parallel.
…ved from BALs, there's not much gain from having a BAL state prefetcher
…ulation in statedb across multiple calls to Finalise. instead, Finalise now returns the diff that occurred from that call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far, just a few comments.
var resCh chan *ProcessResultWithMetrics | ||
resCh, err = bc.processor.ProcessWithAccessList(block, statedb, bc.cfg.VmConfig) | ||
if err != nil { | ||
// TODO: okay to pass nil here as execution result? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine, we just use it to get out the receipts (if it is non nil).
…erdam. Enable access list generation in miner post-amsterdam
We decided on the EIP breakout call to keep account/storage reads in for now. Basically, without having this information it is possible that a transaction can be crafted which does a ton of storage reads and slow down the processing of the block significantly. So until we can evaluate worst-cases here, account/storage reads are staying in the spec. |
bc.prefetcher.Prefetch(block, throwaway, vmCfg, &interrupt) | ||
if block.Body().AccessList == nil { | ||
// only use the state prefetcher for non-BAL blocks. | ||
bc.prefetcher.Prefetch(block, throwaway, vmCfg, &interrupt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I previously implemented a state prefetcher for BALs. I deleted it so that the import benchmarks I am running will capture the BAL block processing speed equivalent to a synced node executing blocks at chain head.
We should probably add back the BAL prefetcher and measure the import performance.
Also TODO: need to implement validation of storage/account reads against the BAL. |
@@ -118,6 +120,14 @@ type StateDB struct { | |||
// The tx context and all occurred logs in the scope of transaction. | |||
thash common.Hash | |||
txIndex int | |||
sender common.Address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove trhis.
@@ -118,6 +120,14 @@ type StateDB struct { | |||
// The tx context and all occurred logs in the scope of transaction. | |||
thash common.Hash | |||
txIndex int | |||
sender common.Address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover, remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be committed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's leftover test data for a unit test that no longer exists (but should be updated and added back in).
WIP. currently passes blockchain spec tests (tests that make use of balances >16 bytes are excluded).
Change summary:
--experimentalbal
is enabled:Deviations from EIP-7928:
txindex=0
correspond to state reads/changes which occur from system contract execution before transactions.txindx=1..len(block.transactions)
correspond to state reads/changes occurring for each transaction.txindex=len(block.transactions)+1
correspond to the post-block system contracts and EIP-4895 withdrawals.TODO: